Plugins/classlib: Support multiple RandIDs in one SynthDef #6159
Conversation
jamshark70
force-pushed
the
topic/MultipleRandIDs
branch
from
e72eb1b
to
5dbfa67
Compare
|
Thanks @telephon for approving -- I just added two unit tests:
|
jamshark70
force-pushed
the
topic/MultipleRandIDs
branch
from
5dbfa67
to
83f813a
Compare
|
Urgent reminder of #2363 - adding UGen arguments on the server side breaks the binary API and potentially crashes clients and earlier written SynthDefs (accessing unallocated memory). I'd appreciate if this issue was always strongly on the mind when considering editing UGens. We currently don't have a safe way to handle this. Also, an API change marker should be added to these commits should they be merged. |
server/plugins/NoiseUGens.cpp
Outdated
| @@ -790,9 +790,13 @@ void RandID_Ctor(RandID* unit) { | |||
|
|
|||
| void RandID_next(RandID* unit, int inNumSamples) { | |||
| float id = ZIN0(0); | |||
| bool fire = ZIN0(1) > 0; | |||
There was a problem hiding this comment.
is there a way we can know the number of allocated UGen inputs, so we could write
bool fire = num_ugen_args >= 2 && ZIN0(1) > 0Then we would avoid crashing against clients and synth-defs using the previous ABI.
There was a problem hiding this comment.
Christof noted that there is already a numInputs that could/should be used here.
There was a problem hiding this comment.
How about this?
bool fire = (unit->mNumInputs >= 2 ? ZIN0(1) : 0.f) > 0.f;
There was a problem hiding this comment.
Should it not just be
bool fire = unit->mNumInputs >= 2 && (ZIN0(1) > 0.f);?
There was a problem hiding this comment.
bool fire = unit->mNumInputs >= 2 && (ZIN0(1) > 0.f);
OK, done, passes an inputs-mismatch test.
|
Agreed with sciss that these changes should be un-approved until the problem is resolved. |
In this case, I could not reproduce a crash. It's correct that we should keep this in mind (and my fault that I didn't think of it). It would also be good to understand the boundary conditions better (e.g., maybe adding a kr input is fine but adding an ar is not fine). |
Purpose and Motivation
See https://scsynth.org/t/different-randseed-for-the-same-synthdef/7248/27, where it was desired to use more than one RNG ID in the same SynthDef. Prior to this change, this was impossible because RandID would set the ID only when the ID input changed. This use case requires setting the ID on every control block, once for one part of the graph, and again for the other part.
However, losing the old behavior would also not be desirable.
So this change adds a new input to RandID,
force.force <= 0(false), RandID behaves the old way, looking for changes in the input.force > 0(true), RandID will set the ID in every control block, whether it changed or not.The default is the old way.
As with a couple of other recent PRs, I would prefer not to have to write a unit test until we are reasonably sure this will be merged. So a unit test isn't yet provided. The approach would be to generate two separate, identically-seeded random number streams and make sure they are the same.
Types of changes
To-do list